-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatically open and copy the menu on public link share activation #11537
Conversation
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv definitely looks better than currently! :) 👍 (As discussed, let’s try with "Copy" for text next to the copy icon.) |
@jancborchardt I have no time for this one ^^ |
@skjnldsv I’ll try and look into it. :) |
@skjnldsv are you aware of #11169 and #11844 I have the feeling that we duplicate work here and changing the same stuff at the same time in two different pull request. Maybe you want to join me on my PR and while making multiple links possible improve the UX. Your JS experience would be much appreciated. You can already find some suggestions in the issue linked here. |
@schiessle please see my comment at #11169 (comment) – these two issues are actually separate, and the "multiple link shares" addition should not change the entire flow of how links work. There is a spec we designed which defines it, and if we throw around stuff midway, that’s recipe for confusion and breakage. |
@jancborchardt but we touch and change exactly the same code which creates a hell of merge conflicts. Let's implement the multiple share links and while touching the code anyway make it work smooth. |
@schiessle yes, but as we defined in design discussions with multiple people. 🙂 Also, this is a bugfix we clearly need to backport since we had so many complaints already. And multiple link sharing is a feature. So we need to keep it separate. |
Thanks for working on this, last upgrade made me wonder what was happening. I'd just point a few things though:
|
@skjnldsv @jancborchardt What if we dump the checkbox? |
I’m not super excited about this – checkboxes are a very clear indicator of state. Having our own standard for color & text needs people to read text and be able to view color. We just need to integrate the checkbox nicely, like move it slightly over the icon maybe. |
Have you guys considered just having the text be a link to open the share menu for the item? I have about 5 users that complain constantly about how that UI is very misleading like they want to click the text or the icon to get to the menu. but instead they have to home in on the Kabob which is fine for when you want to do more than just share. |
The new spec is at #11844 (comment) The only issue is that this will then only land for Nextcloud 15 and not be backportable. Which could be fine because the development cycle is almost over already. cc @skjnldsv @jospoortvliet |
@mcarpenterjr I added a point in my comment of the spec:
This would maybe take care of that? Btw @skjnldsv feel free to close this when it’s rolled into #11844 |
@jancborchardt Thanks man, looks great. |
@nextcloud/designers
I really dislike what I have done right here, feels terrible!
Is there no way of waiting for a dom element to appear?
@jancborchardt Or we can find another way to fix this issue